Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor: Add array / array sqllogic tests for array_contains #6771

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Minor: Add array / array sqllogic tests for array_contains #6771

merged 3 commits into from
Jun 28, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jun 26, 2023 •
edited
Loading

Which issue does this PR close?

Related to #6709

Rationale for this change

While reviewing #6618 I felt there was a gap in coverage:

What changes are included in this PR?

tests for array functions on array functions

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jun 26, 2023
@alamb alamb mentioned this pull request Jun 26, 2023
Merged
@izveigor
Copy link
Contributor

Is there any idea to do something similar with math, string, time functions? 🤔
Sources:
https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/math.slt
https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/scalar.slt
https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/strings.slt

@alamb
Copy link
Contributor Author

alamb commented Jun 27, 2023 •
edited
Loading

Is there any idea to do something similar with math, string, time functions? 🤔

We should for sure -- I wonder if we can leverage existing .slt files (from sqllite, for example). It would be a good thing / gap to track with tickets

@alamb alamb force-pushed the alamb/array_contains_tests branch from 6be5869 to dd40420 Compare June 27, 2023 18:40
@github-actions github-actions bot removed logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Jun 27, 2023
@alamb alamb marked this pull request as ready for review June 27, 2023 18:41
@alamb alamb changed the title Add array / array sqllogic tests for array_contains Minor: Add array / array sqllogic tests for array_contains Jun 27, 2023
izveigor
izveigor reviewed Jun 28, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/array.slt

# array_contains array

statement ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to test nested arrays even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in eb6e88f

@izveigor izveigor mentioned this pull request Jun 28, 2023
Merged
@alamb alamb merged commit 6c8df1a into apache:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@izveigor izveigor izveigor left review comments

Assignees
No one assigned
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alamb @izveigor